-
Notifications
You must be signed in to change notification settings - Fork 10
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Name option #147
base: master
Are you sure you want to change the base?
Name option #147
Conversation
src/omero_zarr/cli.py
Outdated
default="id", | ||
choices=["id", "name"], | ||
help=( | ||
"How to name the Image or Plate zarr. Default 'id' is [ID].zarr. " |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That raises the question about the extension.
In both cases, the library is used to generate ome.zarr
so in one case using .zarr
and in the other case .ome.zarr
is confusing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In general I think we've stated elsewhere:
- strongly SHOULD (bordering on MUST):
.zarr
- SHOULD:
.ome.zarr
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@joshmoore You're suggesting we update the default behaviour to be [ID].ome.zarr
instead of [ID].zarr
?
Always using .ome.zarr
(for name.ome.zarr or ID.ome.zarr) is more consistent, so I'm happy to do that, but that is more of a breaking change
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Breaking in the sense that you think current processes won't detect that something has already been exported and will re-export?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, just if you've got a workflow like:
$ omero zarr export Image:1
$ mv 1.zarr renamed_image.zarr
that workflow will need to change because 1.zarr
will now be created as 1.ome.zarr
But that's not a major concern, so happy to make that change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 I'm personally a bit up in the air. A benefit of .zarr
filesets are that other stuff can be stored in them which is why I don't think we should ever require .ome.zarr
. On the flip side, what we do now will likely be the model that other submitters to BIA tend to follow, so it does make sense to do what we think is most strategic (and update any existing workflows as necessary). All that being said, 💯 for doing whatever consistently.
I have reverted the removal of
But exporting an Image with a more complex name, e.g.
I don't know a good way to make all names "safe" from these types of errors. Another example that fails with a different Error has Image name
Same error with:
and
But, removing the Other examples that work OK:
|
So it looks like all names are OK except for those with |
Replacing |
👍 Looks good to me. I've used the build from this branch a few times already for the NGFF conversion/export work. |
src/omero_zarr/raw_pixels.py
Outdated
name = os.path.join(target_dir, "%s.zarr" % plate.id) | ||
name_by = args.name_by | ||
|
||
if name_by == "name": |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Two quick notes:
- this code is duplicated and could be made a method for possible sub-classing
- I can live with this split in naming schemes, but it could be a surprise
@joshmoore I reduced duplication by creating I also noticed that we need the name for polygon/masks export, but supporting the |
74855f7
to
470ad2b
Compare
Anything else needed here? |
Nothing outstanding from my side. |
This PR adds an optional
name_by
argument with options ofid
(default behaviour) andname
. It is needed for batch exporting many Images or Plates where we want the exported OME-Zarr image to have a useful name.This PR has been used for all the
omero-cli-zarr
exports for ongoing IDR NGFF upgrade work:When exporting from OMERO, we now adopt the naming convention of
ID.ome.zarr
orPlateName.ome.zarr
instead of the previousID.zarr
.If names contains square brackets
[ ]
then this can break writing to zarr (see errors below) so these are replaced by( )
.To test: